EZP-29518: ContentType option "Default field for sorting children" has no effect on new created objects #2428

mateuszbieniek · 2018-08-23T10:51:43Z

Question	Answer
JIRA issue	EZP-29518
Bug	yes
New feature	no
Target version	`6.7+`
BC breaks	no
Tests pass	yes
Doc needed	no

Setting ContentType option "Default field for sorting children" had no effect on newly created Content Object of said ContentType - they were always set to default "Content Name Ascending". This PR fixes it.

TODO:

Implement feature / fix a bug.
Implement/fix tests.
Ask for Code Review.

TODO ON MERGE

Adapt SiteAccessAware Repo when merging to 2.2

mateuszbieniek · 2018-08-23T10:52:31Z

This is followup for #2421 as it got closed automatically after rebase.

alongosz · 2018-08-27T11:40:50Z

eZ/Publish/Core/REST/Client/LocationService.php

@@ -80,16 +81,20 @@ public function setSession($id)
     * Instantiates a new location create class.
     *
     * @param mixed $parentLocationId the parent under which the new location should be created
+     * @param ContentType|null $contentType


Please use FQCN (applies to other places in this PR as well).

mateuszbieniek · 2018-08-27T13:08:20Z

Changes requested by @alongosz completed + additional code: missing failsafe for not implemented sort fields. Ping @andrerom, @konradoboza.

alongosz · 2018-08-27T13:11:51Z

eZ/Publish/Core/Repository/ContentService.php

@@ -823,6 +824,14 @@ protected function buildSPILocationCreateStructs(array $locationCreateStructs)
                );
            }

+            if (!isset(Location::SORT_FIELD_MAP[$locationCreateStruct->sortField])) {
+                $locationCreateStruct->sortField = Location::SORT_FIELD_NAME;


Personally I would throw in this case, but that might be rather for kernel 8.x due to BC.

yes, more of a 8.x way

This is failsafe for when someone set it to not implemented sortField - which did nothing prev this PR - and after updating will create the new object of said ContentType, which now would break.

andrerom · 2018-08-29T05:40:08Z

@mateuszbieniek Could you fix the PHP 5 incompatibility here so we can send it to QA?
ref: PHP Fatal error: Cannot use isset() on the result of an expression (you can use "null !== expression" instead) in /home/travis/build/ezsystems/ezpublish-kernel/eZ/Publish/Core/Repository/ContentService.php on line 827

mateuszbieniek · 2018-08-29T07:43:00Z

@andrerom done.

andrerom · 2018-09-06T03:57:39Z

eZ/Publish/Core/REST/Client/LocationService.php

+            $properties['sortField'] = $contentType->defaultSortField;
+            $properties['sortOrder'] = $contentType->defaultSortOrder;
+        }
+        return new LocationCreateStruct($properties);


hmm, not a blocker for QA here but seems CS check for some reason did not spot this (cs/travis conf wrong on 6.7?)
Afaik our CS profile dictates a empty line before return statements, right @alongosz ?

Hmm, indeed fixer should complain here. Locally it does.

barbaragr · 2018-09-17T13:55:36Z

Tested with 1.7 seems OK, but I've got 2 questions:
Question 1:

Clean installation of eZ Platform 2.2
Create new Content Type named "Test Content Type", set Default field for sorting children to Location Priority and save it.
Create a new Content Object of "Test Content Type" type. Publish it.
Go to the "Details" tab of the newly created Content Object.
Result: In "Sub-items sorting order" section you will see that sub-items are ordered by "Priority", but on the list when you can choose sorting ordering (on Details tab), there are only 4 items. When you create Content Type you can choose from 9 items. Article has got 5 items to select with.
Content Type:

Test Content Type:

Article:

Do we accept those differences?

Question 2:

Clean installation of eZ Platform 2.2
Create new Content Type named "Test Content Type", set Default field for sorting children to Location Priority and save it.
Create a new Content Object of "Test Content Type" type - "First". Publish it.
Go to the "Details" tab of the newly created Content Object - sorting by Priority is set.
Go to edit "Test Content Type" and change default sorting for Content Name and save it
Create a new Content Object of "Test Content Type" type - "Second". Publish it.
Go to the "Details" tab of the newly created Content Object - sorting by Name is set, but if you go back to the "First" content item, sorting by priority is still set. Shouldn't it be refreshed?

mateuszbieniek · 2018-09-18T12:56:56Z

@barbaragr
1: this is out of the scope of this PR - not all sort fields that you can choose when editing/creating Content Type are implemented and even less are registered to be shown when selecting sort field for existing Content Object. I will create another PR that will fix that.

2: it should not be refreshed. Sorting field set in Content Type is a default one ONLY for newly created Content Object. Changing it changes sort field only for Content Objects that would be created afterward. This is the correct and expected behavior.

lserwatka · 2018-09-24T11:49:26Z

@mateuszbieniek rebase is needed.

…ng failsafe.

mateuszbieniek · 2018-09-25T10:49:10Z

Done @lserwatka

lserwatka · 2018-09-25T10:57:23Z

You can merge it up

mateuszbieniek · 2018-09-26T08:36:14Z

@lserwatka done

andrerom · 2018-09-26T08:46:13Z

Thanks for fixing this @mateuszbieniek, nice improvment of kernel 👍 🎉

mateuszbieniek mentioned this pull request Aug 23, 2018

[WIP] EZP-29518: ContentType option "Default field for sorting children" has no effect on new created objects #2421

Closed

3 tasks

mateuszbieniek requested review from kmadejski, andrerom and alongosz and removed request for kmadejski August 23, 2018 11:09

ezsystems deleted a comment from ezrobot Aug 23, 2018

mateuszbieniek force-pushed the EZP-29518 branch from 437b855 to 33d31f4 Compare August 23, 2018 12:41

ezsystems deleted a comment from ezrobot Aug 23, 2018

andrerom approved these changes Aug 23, 2018

View reviewed changes

andrerom changed the title ~~[WIP] EZP-29518: ContentType option "Default field for sorting children" has no effect on new created objects~~ EZP-29518: ContentType option "Default field for sorting children" has no effect on new created objects Aug 23, 2018

mateuszbieniek requested a review from konradoboza August 27, 2018 07:19

konradoboza approved these changes Aug 27, 2018

View reviewed changes

alongosz requested changes Aug 27, 2018

View reviewed changes

alongosz approved these changes Aug 27, 2018

View reviewed changes

konradoboza approved these changes Aug 27, 2018

View reviewed changes

andrerom added the Ready for QA label Sep 6, 2018

andrerom reviewed Sep 6, 2018

View reviewed changes

barbaragr self-assigned this Sep 17, 2018

barbaragr added QA approved and removed Ready for QA labels Sep 24, 2018

barbaragr approved these changes Sep 24, 2018

View reviewed changes

mateuszbieniek added 2 commits September 25, 2018 10:25

Fixed ContentType 'Default field for sorting children'

7f7a9cd

Fixed broken test and CS

dd6b57a

mateuszbieniek added 3 commits September 25, 2018 10:26

Fixed not using FQCN for method parameters in PHP doc and added missi…

5602afd

…ng failsafe.

Fixed 'Cannot use isset() on the result of an expression' error

54ffb25

Added missing empty line before return statement.

03227c6

mateuszbieniek force-pushed the EZP-29518 branch from b70df57 to 03227c6 Compare September 25, 2018 08:30

lserwatka merged commit 73d0544 into ezsystems:6.7 Sep 25, 2018

mateuszbieniek mentioned this pull request Oct 24, 2018

EZP-29518: ContentType option "Default field for sorting children" has no effect on new created objects ezsystems/repository-forms#258

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-29518: ContentType option "Default field for sorting children" has no effect on new created objects #2428

EZP-29518: ContentType option "Default field for sorting children" has no effect on new created objects #2428

mateuszbieniek commented Aug 23, 2018 •

edited by andrerom

Loading

mateuszbieniek commented Aug 23, 2018

alongosz Aug 27, 2018

mateuszbieniek commented Aug 27, 2018

alongosz Aug 27, 2018

andrerom Aug 29, 2018

mateuszbieniek Aug 29, 2018

andrerom commented Aug 29, 2018

mateuszbieniek commented Aug 29, 2018

andrerom Sep 6, 2018 •

edited

Loading

alongosz Sep 6, 2018

barbaragr commented Sep 17, 2018

mateuszbieniek commented Sep 18, 2018

lserwatka commented Sep 24, 2018

mateuszbieniek commented Sep 25, 2018

lserwatka commented Sep 25, 2018

mateuszbieniek commented Sep 26, 2018

andrerom commented Sep 26, 2018

EZP-29518: ContentType option "Default field for sorting children" has no effect on new created objects #2428

EZP-29518: ContentType option "Default field for sorting children" has no effect on new created objects #2428

Conversation

mateuszbieniek commented Aug 23, 2018 • edited by andrerom Loading

mateuszbieniek commented Aug 23, 2018

alongosz Aug 27, 2018

Choose a reason for hiding this comment

mateuszbieniek commented Aug 27, 2018

alongosz Aug 27, 2018

Choose a reason for hiding this comment

andrerom Aug 29, 2018

Choose a reason for hiding this comment

mateuszbieniek Aug 29, 2018

Choose a reason for hiding this comment

andrerom commented Aug 29, 2018

mateuszbieniek commented Aug 29, 2018

andrerom Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

alongosz Sep 6, 2018

Choose a reason for hiding this comment

barbaragr commented Sep 17, 2018

mateuszbieniek commented Sep 18, 2018

lserwatka commented Sep 24, 2018

mateuszbieniek commented Sep 25, 2018

lserwatka commented Sep 25, 2018

mateuszbieniek commented Sep 26, 2018

andrerom commented Sep 26, 2018

mateuszbieniek commented Aug 23, 2018 •

edited by andrerom

Loading

andrerom Sep 6, 2018 •

edited

Loading